Skip to content

[WIP] restore .py file extensions to the tensorflow files #4331

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

[WIP] restore .py file extensions to the tensorflow files #4331

wants to merge 6 commits into from

Conversation

syrull
Copy link

@syrull syrull commented Apr 11, 2021 •
edited by dhruvmanila
Loading

Describe your change:

Whenever Tensorflow is released for python 3.9 (tensorflow/tensorflow#44485) ensure that the tests are passing.

fixes: #3946

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Documentation change?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new Python files are placed inside an existing directory.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • All functions and variable names follow Python naming conventions.
  • All function parameters and return values are annotated with Python type hints.
  • All functions have doctests that pass the automated testing.
  • All new algorithms have a URL in its comments that points to Wikipedia or other similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

@syrull syrull requested a review from Kush1101 as a code owner April 11, 2021 22:57
@ghost ghost added enhancement This PR modified some existing files awaiting reviews This PR is ready to be reviewed tests are failing Do not merge until tests pass labels Apr 11, 2021
@syrull syrull changed the title restore .py file extensions to the files (#3946) restore .py file extensions to the tensorflow files Apr 11, 2021
@dhruvmanila
Copy link
Member

dhruvmanila commented Apr 27, 2021 •
edited
Loading

@f0xxx1 Oh, there's one more PR to do the same job. I will keep this open. One more change which needs to be done is to remove the python < 3.9 condition from requirements.txt for tensorflow and keras. Then, the tests should pass and this will be merged.

@dhruvmanila dhruvmanila reopened this Apr 27, 2021
@dhruvmanila
Copy link
Member

Ops, sorry for that.

@dhruvmanila dhruvmanila self-assigned this Apr 27, 2021
This was referenced Apr 28, 2021
Closed
Closed
@cclauss
Copy link
Member

cclauss commented Apr 28, 2021

I believe that this will fail our tests because of #3946

@dhruvmanila
Copy link
Member

I think there should only be one PR to fix #3946 and the rest should be closed in favor of it. This should be that PR as it already has the renaming part and we only need to test it after we enable tensorflow.

@dhruvmanila dhruvmanila mentioned this pull request Apr 28, 2021
Closed
14 tasks
@dhruvmanila
Copy link
Member

Oh, completely missed that support for Python 3.9 is only added for the release candidate version. I will test it out whether it works or not for that version.

@dhruvmanila dhruvmanila changed the title restore .py file extensions to the tensorflow files [WIP] restore .py file extensions to the tensorflow files Apr 28, 2021
@dhruvmanila
Copy link
Member

Awesome, it's working now! Just needs to fix those pre-commit error and once the library is released, this should be good to go :)

@f0xxx1 Can you run pre-commit locally and fix those errors?

@syrull
Copy link
Author

syrull commented May 6, 2021

@dhruvmanila I tried to fix the pre-commit errors however I don't think that black/flake8 are being compatible with the current configurations.

I made some changes, but I think that I have to change way too much in order for it to work. Here is the output of the pre-commit

flake8...................................................................Failed
- hook id: flake8
- exit code: 1

scripts/validate_solutions.py:76:22: E701 multiple statements on one line (colon)
scripts/validate_solutions.py:76:22: E231 missing whitespace after ':'
scripts/validate_solutions.py:76:23: E225 missing whitespace around operator
dynamic_programming/k_means_clustering_tensorflow.py:39:9: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:41:9: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:42:9: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:46:9: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:47:9: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:53:9: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:54:9: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:56:9: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:57:9: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:63:9: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:70:9: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:76:9: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:77:9: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:82:9: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:84:9: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:85:9: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:86:9: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:87:9: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:93:9: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:101:13: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:102:13: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:103:13: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:109:17: E265 block comment should start with '# '
dynamic_programming/k_means_clustering_tensorflow.py:125:13: E265 block comment should start with '# '

Should I go for it or ?

@cclauss
Copy link
Member

cclauss commented May 6, 2021 •
edited
Loading

Please correct the flake8 issues.

Black did a new release in the past 48 hours so please ensure that .pre-commit is using the latest black release.

cclauss
cclauss reviewed May 6, 2021
View reviewed changes
cclauss
cclauss reviewed May 6, 2021
View reviewed changes
neural_network/input_data.py
Comment on lines +323 to +324
f"Validation size should be between 0 "
f"and {len(train_images)}. Received: {validation_size}."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
f"Validation size should be between 0 "
f"and {len(train_images)}. Received: {validation_size}."
"Validation size should be between 0 and "
f"{len(train_images)}. Received: {validation_size}."

cclauss
cclauss reviewed May 6, 2021
View reviewed changes
neural_network/input_data.py
@@ -46,16 +44,16 @@ def _read32(bytestream):
def _extract_images(f):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single letter variable names make the code look old school. Would it be possible upgrade to more self-documenting variable names and add type hints?

@cclauss
Copy link
Member

cclauss commented May 17, 2021

Please resolve conflicts because tensorflow v2.5 is shipping and #4422 has landed.

@cclauss cclauss closed this Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cclauss cclauss cclauss left review comments

@Kush1101 Kush1101 Awaiting requested review from Kush1101

Assignees

@dhruvmanila dhruvmanila

Labels
awaiting reviews This PR is ready to be reviewed enhancement This PR modified some existing files tests are failing Do not merge until tests pass
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reenable files when TensorFlow supports the current Python
4 participants
@syrull @dhruvmanila @cclauss @pwnsyl